Create Unit tests for Pyodide #697

leandro-benedet-garcia · 2024-08-21T12:30:32Z

Hello, as I mentioned in #687 I am creating unit tests for Pyodide, however, I am creating as a draft because I am going to actually need some assistance as I don't know very well this code base.

Currently I am going trough the route of pybind11 with a pyproject.toml, did some changes in the Cmake file of nanobind in the tests, but for some reason, every time I try to build the tests, I get this error:

FAILED: test_ndarray_ext.pyi /tmp/tmpnfzv8k08/build/test_ndarray_ext.pyi 
cd /tmp/tmpnfzv8k08/build && /tmp/build-env-1tz5s2os/bin/python /home/leandrobg/Builds/nanobind/src/stubgen.py -q -i /tmp/tmpnfzv8k08/build -m test_ndarray_ext -o test_ndarray_ext.pyi
Traceback (most recent call last):
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1411, in <module>
    main()
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1344, in main
    mod_imported = importlib.import_module(mod)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1324, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'test_ndarray_ext'
[47/83] Generating test_classes_ext.pyi
FAILED: test_classes_ext.pyi /tmp/tmpnfzv8k08/build/test_classes_ext.pyi 
cd /tmp/tmpnfzv8k08/build && /tmp/build-env-1tz5s2os/bin/python /home/leandrobg/Builds/nanobind/src/stubgen.py -q -i /tmp/tmpnfzv8k08/build -m test_classes_ext -o test_classes_ext.pyi
Traceback (most recent call last):
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1411, in <module>
    main()
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1344, in main
    mod_imported = importlib.import_module(mod)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1324, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'test_classes_ext'
[48/83] Generating test_functions_ext.pyi
FAILED: test_functions_ext.pyi /tmp/tmpnfzv8k08/build/test_functions_ext.pyi 
cd /tmp/tmpnfzv8k08/build && /tmp/build-env-1tz5s2os/bin/python /home/leandrobg/Builds/nanobind/src/stubgen.py -q -i /tmp/tmpnfzv8k08/build -m test_functions_ext -o test_functions_ext.pyi
Traceback (most recent call last):
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1411, in <module>
    main()
  File "/home/leandrobg/Builds/nanobind/src/stubgen.py", line 1344, in main
    mod_imported = importlib.import_module(mod)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/usr/lib/python3.12/importlib/__init__.py", line 90, in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "<frozen importlib._bootstrap>", line 1387, in _gcd_import
  File "<frozen importlib._bootstrap>", line 1360, in _find_and_load
  File "<frozen importlib._bootstrap>", line 1324, in _find_and_load_unlocked
ModuleNotFoundError: No module named 'test_functions_ext'

Which, I don't know if I am doing something wrong, or a bug, because I don't see anything special related to Pyodide/Emscripten in the Cmake test in pybind11

Pyodide tests

henryiii · 2024-08-21T20:54:49Z

@leandro-benedet-garcia Could you give me access to push to your fork? I've got this approach working, at least locally. Or you can just pull the changes yourself from https://github.com/henryiii/nanobind @ henryiii/fix/standalonetests.

leandro-benedet-garcia · 2024-08-22T00:48:25Z

@henryiii I added you to the fork, I tried with just pull request but in the CI it showed the same error.

henryiii · 2024-08-22T03:51:03Z

Ahhh, good point, I can just run the CI on my fork first. The problem you are seeing is that you can't import the modules you are building during the compilation phase when cross-compiling. Only later via WASM. So we need to be able to disable the subgen tests. (Well, easily, anyway. There are ways to set the cross-compiling emulator in CMake)

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

hoodmane · 2024-08-22T07:10:43Z

@leandro-benedet-garcia can you add me too?

hoodmane · 2024-08-22T07:12:00Z

Actually it seems to work now.

wjakob · 2024-08-22T13:19:08Z

tests/CMakeLists.txt

+  endif()
+  install(
+    TARGETS
+      inter_module


There is a bit too much repetition of test names in this project. How about defining them as a list and then creating the build and install targets by pasting the list or iterating over elements.

I love list(TRANSFORM (CMake 3.12+) :)

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

leandro-benedet-garcia · 2024-08-22T15:01:27Z

Allrighty, since it seems to be working now I will put it for review, thank you @henryiii

henryiii · 2024-08-22T16:16:12Z

Let me know if you have questions about any of the changes! Very high level, I made the find_package(Python) GLOBAL, since it was not usable outside the directory it's triggered from, but the nanobind_* functions are. I changed Python_INCLUDE_DIRS into using Python::Module's INTERFACE_INCLUDE_DIRS entirely to get an error if it's not visible in that scope; before it would just leave it empty. Making a FATAL_ERROR if Python_INCLUDE_DIRS is undefined would do the same thing. All path variables in CMake should be surrounded by quotes, I noticed them missing in various places, but didn't fix them all for now. I added -fexceptions if compiling for Emscripten, since it's required (and pybind11 does that now too). Tests support being built directly standalone. The stubs are not generated if cross-compiling. I've disabled the stub tests on the emscripten platform. And the stub tests now look up the stubs relative to the compiled modules, rather than the tests.

If you want to try out out locally without Emscripten, you can do something like this:

pipx run build --wheel
uv venv
uv pip install dist/nanobind_tests-0.0.1-cp312-cp312-macosx_14_0_x86_64.whl
py -m pytest

wjakob · 2024-08-28T15:23:54Z

What's the status of this PR? Anything left to do, should I review?

leandro-benedet-garcia · 2024-08-28T15:25:09Z

What's the status of this PR? Anything left to do, should I review?

it is ready for review.

wjakob

A first round of feedback / questions.

One high level thing that I did not like is how this change seems to elevate the tests/CMakeLists.txt into its own top-level makefile that recursively includes the parent. It seems hacky to me and makes things more difficult to understand when reading through the CMake code.

What's special about Emscripten that it cannot follow the previous pattern?

wjakob · 2024-08-28T15:28:05Z

CMakeLists.txt

@@ -155,7 +155,8 @@ if (NOT TARGET Python::Module OR NOT TARGET Python::Interpreter)

  find_package(Python 3.8
    REQUIRED COMPONENTS Interpreter ${NB_PYTHON_DEV_MODULE}
-    OPTIONAL_COMPONENTS Development.SABIModule)
+    OPTIONAL_COMPONENTS Development.SABIModule
+    GLOBAL)


this GLOBAL parameter exists starting in CMake 3.24, while this file declares

cmake_minimum_required(VERSION 3.15...3.27)

What are the implications?

I'm going to try to experiment a bit here in a day or two and see if I can either make the test subdirectory build nicer or build from the parent directory.

wjakob · 2024-08-28T15:29:29Z

cmake/nanobind-config.cmake

@@ -228,9 +233,10 @@ function (nanobind_build_library TARGET_NAME)
      ${NB_DIR}/ext/robin_map/include)
  endif()

+  get_property(nanobind_python_headers TARGET Python::Module PROPERTY INTERFACE_INCLUDE_DIRECTORIES)


Following the style of the rest of the code, how about renaming this to NB_PYTHON_HEADERS?

I would also like to know: under what conditions does the new code lead to behavior that is different from what was there before?

If the headers and targets are not defined, then this produces an error, while the other one silently adds an empty string and proceeds to fail much later with a Python.h not found error.

wjakob · 2024-08-28T15:32:13Z

cmake/nanobind-config.cmake

@@ -275,6 +281,9 @@ function (nanobind_compile_options name)
  if (MSVC)
    target_compile_options(${name} PRIVATE $<$<COMPILE_LANGUAGE:CXX>:/bigobj /MP>)
  endif()
+  if (CMAKE_SYSTEM_NAME MATCHES Emscripten)


Is this line needed? We already declared this as a PUBLIC exported flag of the nanobind library target, which is also inherited by anything else consuming the target.

wjakob · 2024-08-28T15:33:15Z

tests/CMakeLists.txt

@@ -1,11 +1,32 @@
+include_guard(GLOBAL)


This line seems superfluous, nobody will double-include this file.

wjakob · 2024-08-28T15:35:02Z

tests/CMakeLists.txt

@@ -1,11 +1,32 @@
+include_guard(GLOBAL)
+
+cmake_minimum_required(VERSION 3.15...3.27)


I'm confused about these lines. It looks to me like this sets up an independent top-level project, but I don't understand why. The tests/CMakeLists.txt file is part of the parent CMakeLists.txt build system.

wjakob · 2024-08-28T15:35:25Z

tests/CMakeLists.txt

+cmake_minimum_required(VERSION 3.15...3.27)
+project(nanobind_tests LANGUAGES CXX)
+
+if(PROJECT_NAME STREQUAL CMAKE_PROJECT_NAME)


This looks complicated to me, and I don't understand why it is needed.

henryiii · 2024-08-28T16:19:38Z

For emscripten, you have to build the test module as a wheel and then test it. There are two ways to do that. In pybind11, we make the test module buildable. The other option would be to have a way to enable tests to be build as the wheel in the outer pyproject.toml. Names can't be changed, so you couldn't have both at the same time, but I think you could use the tests/pyproject.toml but the main CMakeLists.txt - would have to investigate. This would require fewer changes, but I was just updating this PR, which started with the test module buildable (and that does mimic pybind11. But in pybind11 we haven't moved to scikit-build-core yet for the outer build).

I can try the other method in a couple of days.

wjakob · 2024-09-20T01:59:35Z

Any progress on restructuring the PR? Should I close it for now?

leandro-benedet-garcia · 2024-09-20T14:08:45Z

Any progress on restructuring the PR? Should I close it for now?

I will try my hand at it this weekend.

leandro-benedet-garcia and others added 6 commits August 20, 2024 23:32

up for draft

b3afc56

Merge branch 'wjakob:master' into pyodide-tests

8dc1fa4

Merge pull request #1 from leandro-benedet-garcia/pyodide-tests

4d51d18

Pyodide tests

changed gitignore

5a993a9

Merge branch 'wjakob:master' into master

bb3c356

Whoops

4555cd0

henryiii added 4 commits August 22, 2024 01:29

fix: support building tests standalone

b3cd0f7

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

fix: support cross-compiling

def8aef

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

fix: add required flag for emscripten

ec29f2d

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

chore: nicer location for whole archive setting

d7282b9

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

wjakob reviewed Aug 22, 2024

View reviewed changes

chore: reduce duplication in file names

15ebe0f

Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>

henryiii force-pushed the master branch from 630de1f to 15ebe0f Compare August 22, 2024 14:49

leandro-benedet-garcia marked this pull request as ready for review August 22, 2024 15:01

wjakob reviewed Aug 28, 2024

View reviewed changes

wjakob force-pushed the master branch 2 times, most recently from f9e5e0b to 30e96b7 Compare September 9, 2024 14:54

wjakob force-pushed the master branch from 96cca6c to ee23846 Compare September 20, 2024 02:01

wjakob force-pushed the master branch 2 times, most recently from f3e2796 to bff96e2 Compare October 4, 2024 03:20

wjakob force-pushed the master branch from 046c7a1 to e262b7c Compare October 16, 2024 13:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create Unit tests for Pyodide #697

Create Unit tests for Pyodide #697

leandro-benedet-garcia commented Aug 21, 2024

henryiii commented Aug 21, 2024

leandro-benedet-garcia commented Aug 22, 2024

henryiii commented Aug 22, 2024

hoodmane commented Aug 22, 2024

hoodmane commented Aug 22, 2024

wjakob Aug 22, 2024

henryiii Aug 22, 2024

leandro-benedet-garcia commented Aug 22, 2024

henryiii commented Aug 22, 2024 •

edited

Loading

wjakob commented Aug 28, 2024

leandro-benedet-garcia commented Aug 28, 2024

wjakob left a comment

wjakob Aug 28, 2024

henryiii Aug 28, 2024

wjakob Aug 28, 2024

wjakob Aug 28, 2024

henryiii Aug 28, 2024

wjakob Aug 28, 2024

wjakob Aug 28, 2024

wjakob Aug 28, 2024

wjakob Aug 28, 2024

henryiii commented Aug 28, 2024 •

edited

Loading

wjakob commented Sep 20, 2024

leandro-benedet-garcia commented Sep 20, 2024 •

edited

Loading

		@@ -1,11 +1,32 @@
		include_guard(GLOBAL)

		cmake_minimum_required(VERSION 3.15...3.27)

Create Unit tests for Pyodide #697

Are you sure you want to change the base?

Create Unit tests for Pyodide #697

Conversation

leandro-benedet-garcia commented Aug 21, 2024

henryiii commented Aug 21, 2024

leandro-benedet-garcia commented Aug 22, 2024

henryiii commented Aug 22, 2024

hoodmane commented Aug 22, 2024

hoodmane commented Aug 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leandro-benedet-garcia commented Aug 22, 2024

henryiii commented Aug 22, 2024 • edited Loading

wjakob commented Aug 28, 2024

leandro-benedet-garcia commented Aug 28, 2024

wjakob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henryiii commented Aug 28, 2024 • edited Loading

wjakob commented Sep 20, 2024

leandro-benedet-garcia commented Sep 20, 2024 • edited Loading

henryiii commented Aug 22, 2024 •

edited

Loading

henryiii commented Aug 28, 2024 •

edited

Loading

leandro-benedet-garcia commented Sep 20, 2024 •

edited

Loading